GH-50994: [C++][Compute] Implement casting from ListView to List with zero-copy fast-path - #50976
GH-50994: [C++][Compute] Implement casting from ListView to List with zero-copy fast-path#50976Jay846 wants to merge 7 commits into
Conversation
|
Thanks for opening a pull request! This pull request has been automatically converted to a draft because its title doesn't match Arrow's required format. If this is not a minor PR. Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose Opening GitHub issues ahead of time contributes to the Openness of the Apache Arrow project. Then could you also rename the pull request title in the following format? or After updating the title, you can mark the pull request as ready for review. See also: |
Reranko05
left a comment
There was a problem hiding this comment.
Could you use the Arrow PR title template:-
GH-<Issue Number>: [<Component>] <Title>
|
|
|
You should use issue id but not the PR's id. If there isn't an existed issue, you can create a new one. |
|
|
|
Hi @pitrou, I've updated the PR title to match the tracking issue (GH-50994), removed the unused compile variables, and applied clang-format styling. The failures in macOS GLib and Windows/Conda seem to be flaky Flight and S3FS/MinIO test failures. Could we please trigger a re-run of the checks? Thanks! |
Some CI tests are currently failing to run, but this is unrelated to the changes in this PR. Don't worry. |
|
Also pushed a quick style formatting update to satisfy the pre-commit linter check. Kindly approve for re-run. Thanks |
|
https://www.githubstatus.com/ shows that GitHub Actions is currently experiencing some issues. Let's wait for it to recover. |
HuaHuaY
left a comment
There was a problem hiding this comment.
Left a few comments. I'm not sure about the check for in_array.length == 0. The rest looks good to me.
| const ArraySpan& in_array = batch[0].array; | ||
| ArrayData* out_array = out->array_data().get(); | ||
|
|
||
| if (in_array.length == 0) { |
There was a problem hiding this comment.
I may have a mistake during the previous review. I am not sure whether this if condition will always evaluate to false due to the check at cpp/src/arrow/compute/exec.cc:786; perhaps we can assume here that in_array.length is never 0. Let's wait for comments from a reviewer who is more familiar with Arrow Compute.
There was a problem hiding this comment.
Got it, using if constexpr makes perfect sense here. I'll update those two checks.
For the length == 0 condition, I'll leave the check in place for now as a safeguard and wait for input from other maintainers on whether empty batches can reach this execution path. Thanks
Rationale for this change
This PR implements missing type-casting compute kernels to convert
ListViewTypeandLargeListViewTypearrays to standardListTypeandLargeListTypearrays.Previously, these casts routed to
CastList, which ignored the sizes buffer and read offsets out-of-bounds, resulting in corrupted output arrays. This PR introduces a dedicatedCastListViewfunctor to perform correct conversions.What changes are included in this PR?
To maximize performance and optimize memory layouts, a dual-execution path was implemented in the
CastListViewexecution functor insidescalar_cast_nested.cc:offsets[i] + sizes[i] == offsets[i+1]). It avoids copying the child values array entirely, allocating the new output offset buffer, shifting offsets relative to the start, and slicing the child array directly to preserve zero-copy pointer semantics.Int64Builder, and invokes Arrow's internaltakecompute kernel to reconstruct a new contiguous child values array.Are these changes tested?
Yes, added comprehensive unit test suites in
scalar_cast_test.ccpassing all cases. Tests explicitly cover:ListView<int16>toList<int32>).Are there any user-facing changes?
No public API contracts were broken. This adds correct, declarative casting support natively to the existing internal compute framework.